Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add TLS configuration settings/endpoints for auxiliary transports #5152

Draft
wants to merge 24 commits into
base: main
Choose a base branch
from
Draft

Add TLS configuration settings/endpoints for auxiliary transports #5152

wants to merge 24 commits into from

Conversation

finnegancarroll
Copy link
Contributor

@finnegancarroll finnegancarroll commented Mar 5, 2025 •
edited
Loading

Description

Add settings for configuring keystore/truststore resources for optional auxiliary client/server transports in OpenSearch core which are supplied and registered by plugins. For more information regarding auxiliary transports see opensearch-project/OpenSearch#16534.

Initially aux transports will only support client-certificate authentication:
https://opensearch.org/docs/latest/security/authentication-backends/client-auth/

Introduces the following settings for configuring TLS for auxiliary transports:

Enable

plugins.security.ssl.http.enabled
plugins.security.ssl.aux.clientauth_mode
plugins.security.ssl.aux.enabled_ciphers
plugins.security.ssl.aux.enabled_protocols

// will respect the following global settings
plugins.security.disabled
plugins.security.ssl_only

Keystore settings

// for keystores
plugins.security.ssl.aux.keystore_type
plugins.security.ssl.aux.keystore_alias
plugins.security.ssl.aux.keystore_filepath
plugins.security.ssl.aux.keystore_password
// for pemkeys
plugins.security.ssl.aux.pemkey_filepath
plugins.security.ssl.aux.pemkey_password

Truststore settings

// for truststore
plugins.security.ssl.aux.truststore_type
plugins.security.ssl.aux.truststore_alias
plugins.security.ssl.aux.truststore_filepath
plugins.security.ssl.aux.truststore_password
// for pemcerts
plugins.security.ssl.aux.pemcert_filepath
plugins.security.ssl.aux.pemtrustedcas_filepath

TODO: AuthZ will be required for this initial version.

Issues Resolved

#5104

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov Codecov
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 70.51282% with 23 lines in your changes missing coverage. Please review.

Project coverage is 71.68%. Comparing base (305df57) to head (04ba906).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...rg/opensearch/security/ssl/SslSettingsManager.java 79.62% 4 Missing and 7 partials ⚠️
.../opensearch/security/ssl/config/SslParameters.java 21.42% 11 Missing ⚠️
...ensearch/security/ssl/util/SSLConfigConstants.java 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5152      +/-   ##
==========================================
+ Coverage   71.64%   71.68%   +0.04%     
==========================================
  Files         335      335              
  Lines       22748    22803      +55     
  Branches     3599     3607       +8     
==========================================
+ Hits        16297    16346      +49     
- Misses       4651     4655       +4     
- Partials     1800     1802       +2
Files with missing lines Coverage Δ
...arch/security/ssl/OpenSearchSecuritySSLPlugin.java 87.24% <100.00%> (ø)
...a/org/opensearch/security/ssl/config/CertType.java 100.00% <100.00%> (ø)
...opensearch/security/ssl/util/SSLRequestHelper.java 66.98% <100.00%> (ø)
...ensearch/security/ssl/util/SSLConfigConstants.java 80.76% <83.33%> (+1.60%) ⬆️
...rg/opensearch/security/ssl/SslSettingsManager.java 74.66% <79.62%> (+1.58%) ⬆️
.../opensearch/security/ssl/config/SslParameters.java 53.84% <21.42%> (-3.10%) ⬇️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@finnegancarroll finnegancarroll mentioned this pull request Mar 13, 2025
Open
18 tasks
@finnegancarroll finnegancarroll mentioned this pull request Mar 23, 2025
Open
6 tasks
@finnegancarroll
Spotless apply
548cfb8 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Fill in additional post-fix literals with constants.
5f17ca6 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Add aux transport constants.
e61b354 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Add aux to CertType enum.
f563b50 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Load aux settings in SslSettingsManager.
d7d761d 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Comment typos.
14e49f7 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Add SECURITY_SSL_AUX_ENABLE_OPENSSL_IF_AVAILABLE to openSslWarnings.
3eaf0f5 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Consolidate testFailsIfNoConfigDefine tests under single helper.
8e6aee2 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
httpConfigFailsIfHttpEnabledButButNotDefined and transportFailsIfNoCo…
5de6bda 
…nfigDefine(SECURITY_SSL_HTTP_ENABLED) are the same test. Removing dup.

Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Replace httpConfigFailsIfBothPemAndJDKSettingsWereSet with transport …
42f41c7 
…generic helper. Add aux and node-to-node transports.

Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Replace httpConfigFailsIfClientAuthRequiredAndJdkTrustStoreNotSet wit…
79ba93f 
…h generic transport helper. Add aux transport case.

Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Fix error message for validate keystore/pemstore - Print missing sett…
da0113e 
…ing name instead of value.

Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Replace httpConfigFailsIfClientAuthRequiredAndPemTrustedCasNotSet wit…
3ec6124 
…h generic helper. Add aux transport case.

Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Add simple asserts for aux transport to SslSettingsManagerTest.
94d4733 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Update SSLConfigConstants aux constants with new constants.
3ab9921 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Refactor SslSettingsManagerReloadListenerTest to abstract helpers for…
86e4635 
… easier application to each CertType. Add aux transport cases.

Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Refactor SslParameters to load from CertType instead of 'ishttp' bool.
a1d24aa 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Add aux case for Pem and JDK cert loader tests.
ae6edc2 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Abstract getSecureSSLProtocols and getSecureSSLCiphers to handle prov…
3288238 
…ider CertType.

Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Add aux cases for SSLConfigConstantsTest.
c8ad1be 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Implement getSecureAuxTransportSettingsProvider to link with core.
27952da 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Expose aux settings to core through plugin class.
cea7031 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Remove engine from settings provider. Fetch raw params instead.
896a96a 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll
Rebase.
5568a3c 
Signed-off-by: Finn Carroll <[email protected]>
@finnegancarroll finnegancarroll mentioned this pull request Mar 26, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwperks cwperks Awaiting requested review from cwperks cwperks will be requested when the pull request is marked ready for review cwperks is a code owner

@DarshitChanpura DarshitChanpura Awaiting requested review from DarshitChanpura DarshitChanpura will be requested when the pull request is marked ready for review DarshitChanpura is a code owner

@derek-ho derek-ho Awaiting requested review from derek-ho derek-ho will be requested when the pull request is marked ready for review derek-ho is a code owner

@nibix nibix Awaiting requested review from nibix nibix will be requested when the pull request is marked ready for review nibix is a code owner

@peternied peternied Awaiting requested review from peternied peternied will be requested when the pull request is marked ready for review peternied is a code owner

@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 will be requested when the pull request is marked ready for review RyanL1997 is a code owner

@reta reta Awaiting requested review from reta reta will be requested when the pull request is marked ready for review reta is a code owner

@willyborankin willyborankin Awaiting requested review from willyborankin willyborankin will be requested when the pull request is marked ready for review willyborankin is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@finnegancarroll